shared/signal/proto/signalexchange.proto (1)

68-76: LGTM — proper field deprecation and clear semantics.

Reserving tag 9 (previously int64 sessionId) is the correct way to prevent wire-incompatible reuse after the type change to optional bytes sessionId = 10. The new bytes relayServerIP = 11 documentation usefully calls out:

• 4-byte (IPv4) / 16-byte (IPv6) encoding (matches netip.Addr.AsSlice()),
• fallback-only usage,
• SNI/TLS verification still bound to relayServerAddress.

One consideration: also reserved "sessionId" (the old name) alongside the tag, in case any tooling/codegen elsewhere keys off the legacy name. Optional.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/signal/proto/signalexchange.proto` around lines 68 - 76, Add a
symbolic reservation for the old field name to avoid tooling/codegen reusing it:
next to the existing numeric reservation "reserved 9;" add reserved "sessionId";
so that the proto contains both reserved 9; and reserved "sessionId"; while
keeping the new optional bytes sessionId = 10 and bytes relayServerIP = 11
unchanged.

shared/relay/client/dialer/race_dialer.go (1)

46-51: Minor: fluent setter is fine, but document concurrency expectations.

WithServerName mutates the receiver in place. Given RaceDial is constructed per-dial in practice, this is safe, but a one-liner clarifying that RaceDial is not safe for concurrent reconfiguration would prevent future misuse if it's ever cached/shared.

📝 Suggested doc tweak

-// WithServerName sets a TLS SNI/cert validation override. Used when serverURL
-// contains an IP literal but the cert is issued for a different hostname.
+// WithServerName sets a TLS SNI/cert validation override. Used when serverURL
+// contains an IP literal but the cert is issued for a different hostname.
+// Not safe to call concurrently with Dial; configure before invoking Dial.
 func (r *RaceDial) WithServerName(serverName string) *RaceDial {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/relay/client/dialer/race_dialer.go` around lines 46 - 51, The
WithServerName method mutates the RaceDial receiver in place and may be misused
concurrently; update the doc comment for func (r *RaceDial)
WithServerName(serverName string) *RaceDial to state that it mutates the
receiver, is intended to be used on a per-dial RaceDial instance, and is not
safe for concurrent reconfiguration (recommend creating a new RaceDial or
copying before reuse). Also mention that RaceDial is typically constructed
per-dial to make the concurrency expectation explicit.

shared/relay/client/dialer/race_dialer_test.go (1)

31-33: LGTM!

The _ blank identifier on the new serverName parameter is the right choice here since the mock's dialFunc field signature is intentionally not extended — existing tests don't exercise SNI override behavior, and the mock continues to forward only ctx/address.

Worth considering as a follow-up: a dedicated test that asserts RaceDial.WithServerName(...) propagates the override into Dial, since the mock currently ignores it. Optional, not blocking.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/relay/client/dialer/race_dialer_test.go` around lines 31 - 33, The
blank identifier for the new serverName parameter in MockDialer.Dial is fine for
now, but to test SNI override behavior add a follow-up unit test: extend the
mock to capture the serverName (e.g. add or change MockDialer.dialFunc to accept
the serverName or add a separate hook), update MockDialer.Dial to forward the
serverName to that hook, then add a test calling RaceDial.WithServerName(...)
and assert the mock received the expected serverName in Dial; reference
MockDialer.Dial, MockDialer.dialFunc, RaceDial.WithServerName and Dial when
making these changes.

shared/relay/client/dialer/quic/quic.go (1)

26-45: LGTM — SNI override semantics are correct.

Explicit serverName correctly takes precedence over URL-derived SNI, and the default branch preserves the prior IP-literal-skip behavior (avoids invalid SNI for [::1]:443 / 1.2.3.4:443).

One small consideration: if a caller passes serverName that itself is an IP literal, tls.Config.ServerName will be set to an IP. Go's crypto/tls omits the SNI extension in this case while still using the IP for certificate verification. Given the documented contract on RaceDial.WithServerName ("override when serverURL contains an IP literal but cert is issued for a different hostname"), callers are expected to pass an FQDN. An optional sanity check (e.g., skip override if net.ParseIP(serverName) != nil) would harden against misuse, but the code is sound as-is.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/relay/client/dialer/quic/quic.go` around lines 26 - 45, The Dial
function currently unconditionally sets tlsClientConfig.ServerName when
serverName is provided; add a sanity check so we only assign ServerName if
serverName is not an IP literal (use net.ParseIP(serverName) == nil) to avoid
forcing an IP into SNI (which crypto/tls omits); update the switch/case handling
in Dial (look for Dial, serverName, tlsClientConfig.ServerName) to skip the
override when serverName parses as an IP and leave the URL-derived behavior
intact.

shared/relay/client/client.go (1)

380-386: Consider preserving the fallback dial error for diagnostics.

When the primary dial fails and dialFallback also fails, only the primary error is propagated and fbErr is silently discarded. Because dialFallback distinguishes useful operator-facing cases ("no fallback IP configured", substituteHost parse errors, actual fallback dial failure), losing it makes diagnosing why the fallback didn't recover the connection harder in the field.

♻️ Suggested change

 	rd := dialer.NewRaceDial(c.log, dialer.DefaultConnectionTimeout, c.connectionURL, dialers...)
 	conn, err := rd.Dial(ctx)
 	if err != nil {
 		fallbackConn, fbErr := c.dialFallback(ctx, dialers)
 		if fbErr != nil {
-			return nil, err
+			return nil, fmt.Errorf("primary dial: %w; fallback: %v", err, fbErr)
 		}
 		conn = fallbackConn
 	}

Or use errors.Join(err, fbErr) if joining is preferred over wrapping.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/relay/client/client.go` around lines 380 - 386, When primary dial
failure falls back into c.dialFallback, preserve the fallback error instead of
discarding fbErr; in the error path inside the block around c.dialFallback(ctx,
dialers) return a combined error that includes both err and fbErr (e.g., using
errors.Join(err, fbErr) or wrapping fbErr into err with fmt.Errorf) so callers
see why the fallback also failed; update the branch that currently does "if
fbErr != nil { return nil, err }" to return a composed error referencing err and
fbErr while keeping the same return types and behavior when fallback succeeds
and sets conn = fallbackConn.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

shared/relay/client/dialer/race_dialer_test.go (1)

26-33: Optional: consider plumbing serverName into dialFunc for future test coverage.

The mock currently discards the new serverName parameter. That's fine for the existing race-coordination tests, but if/when behavior depends on serverName (TLS SNI override is now part of the dialer contract), tests won't be able to assert it without widening the dialFunc signature. Not a blocker for this PR.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/relay/client/dialer/race_dialer_test.go` around lines 26 - 33, The
MockDialer currently ignores the new serverName parameter; update
MockDialer.dialFunc signature to func(ctx context.Context, address, serverName
string) (net.Conn, error), change the MockDialer.Dial method to accept the
serverName param and pass it through to dialFunc (i.e., return m.dialFunc(ctx,
address, serverName)), and update any test instantiations of MockDialer to
supply a matching dialFunc so tests can assert on serverName (leave protocolStr
unchanged).

shared/relay/client/dialer/ws/ws.go (1)

88-91: Set an explicit MinVersion on the WS TLS config.

Static analysis flagged this block. Since you're already amending tls.Config here to set ServerName, it's a low-cost moment to also pin the minimum TLS version (TLS 1.2 or 1.3) and avoid relying on Go's default, which has shifted between releases.

♻️ Proposed change

 		TLSClientConfig: &tls.Config{
 			RootCAs:    certPool,
 			ServerName: serverName,
+			MinVersion: tls.VersionTLS12,
 		},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/relay/client/dialer/ws/ws.go` around lines 88 - 91, The TLS config in
the WS dialer currently sets RootCAs and ServerName but doesn't pin a minimum
TLS version; update the tls.Config used for TLSClientConfig (in ws.go where
TLSClientConfig is constructed) to include MinVersion: tls.VersionTLS12 (or
tls.VersionTLS13 if you prefer stricter policy) so the client refuses older
protocol versions; ensure you reference the tls.Config used in the WS dialer
initialization (the TLSClientConfig field) when adding this option.

shared/relay/client/client.go (1)

421-447: Minor: SNI with IP-literal serverName won't help cert validation.

When serverURL is rels://[2001:db8::5]:443 (IPv6 literal), substituteHost returns serverName == "2001:db8::5", which WithServerName will set as tls.Config.ServerName. Per RFC 6066 SNI must be a DNS name; Go's TLS stack will then verify the peer cert against that IP via x509.Certificate.VerifyHostname, which generally requires an IP SAN. In practice this fallback path is only exercised when the primary FQDN dial fails, so an IP-literal serverURL shouldn't reach here — but consider rejecting an IP-literal origHost early (or skipping WithServerName in that case) to avoid silently producing an unverifiable TLS config.

Not blocking; flagging since the unit test pins this behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/relay/client/client.go` around lines 421 - 447, substituteHost
currently returns origHost even when it's an IP literal, which leads callers to
set tls.Config.ServerName to an IP (invalid for SNI). Change substituteHost to
detect and treat IP-literal original hosts specially: parse origHost (e.g., via
netip.ParseAddr or net.ParseIP) and if it is an IP literal return an empty
origHost (or otherwise signal “no server name”) so callers will skip
WithServerName; otherwise return the hostname as before. Update substituteHost
to document that an empty origHost means “do not set TLS ServerName.”

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

shared/relay/client/dialer/ws/ws.go (1)

87-92: Consider setting an explicit TLS MinVersion.

The relay server does not explicitly set MinVersion in its TLS configuration, relying on Go's default minimum (TLS 1.2). For consistency and to make security requirements explicit, pin the client to tls.VersionTLS12 in this configuration. This prevents reliance on Go's defaults which could theoretically change in future versions, and ensures compatibility with relay servers that don't enforce TLS 1.3.

♻️ Suggested change

 		TLSConfig: &tls.Config{
 			RootCAs:    certPool,
 			ServerName: serverName,
+			MinVersion: tls.VersionTLS12,
 		},

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/relay/client/dialer/ws/ws.go` around lines 87 - 92, The TLS client
configuration currently sets RootCAs and ServerName but omits an explicit
MinVersion; update the TLSClientConfig (the struct that uses certPool and
serverName) to include MinVersion: tls.VersionTLS12 so the client pins the
minimum TLS version to TLS 1.2 (use the tls package constant tls.VersionTLS12 in
the TLSClientConfig initialization).

shared/relay/client/manager_fallback_test.go (2)

96-145: LGTM on the round-trip flow.

Subtest correctly drives the foreign-relay path: Bob registers on his relay, Alice dials the broken FQDN with Bob's advertised IP and the payload echoes back. The bobSideCh goroutine is safely bounded by ctx (cancelled in t.Cleanup via mCancel), so a t.Fatalf on the Alice side won't permanently leak the reader.

One small nit: the time.After(5*time.Second) timeout here can outlive the parent ctx deadline (20s less time already spent). That's fine in practice but consider <-ctx.Done() instead so any future tightening of the parent timeout propagates.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/relay/client/manager_fallback_test.go` around lines 96 - 145, The test
"fallback IP recovers" uses time.After(5 * time.Second) which can outlive the
parent test context; change the select waiting for bobSideCh to instead listen
on <-ctx.Done() so the parent context deadline/cancellation propagates (look for
the select that reads from bobSideCh in the subtest and replace the time.After
branch with a ctx.Done() branch that fails the test, e.g., t.Fatalf with
ctx.Err()).

---

23-23: Use ephemeral ports to prevent flakiness from port collisions.

The hardcoded ports 52401 and 52402 can cause test failures if another process binds them or if the test runs in parallel with other instances. The same package already provides a freeAddr() helper in client_fallback_test.go that obtains an ephemeral port via net.Listen("tcp", "127.0.0.1:0"). Use this pattern to dynamically allocate ports and read the bound addresses from the servers, avoiding TIME_WAIT collisions and making the test safely repeatable.

Also applies to: 40-40, 82-82

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/relay/client/manager_fallback_test.go` at line 23, Replace the
hardcoded ports in ListenerConfig instances with addresses obtained from the
existing freeAddr() helper used in client_fallback_test.go: call freeAddr() to
get an ephemeral address, assign it to the Address field of
server.ListenerConfig (e.g., where homeCfg := server.ListenerConfig{...} and
other configs are created), and use the bound address returned by the listeners
when starting servers so tests use dynamic ephemeral ports instead of
"127.0.0.1:52401"/"52402"; ensure all three occurrences (the homeCfg creation
and the two other ListenerConfig usages) are updated to use freeAddr() and the
resulting address variables.

shared/signal/client/client.go (1)

74-96: LGTM — payload struct + relay IP encoding.

Gating on IsValid() and using Unmap().AsSlice() is the right choice: an IPv4-mapped IPv6 (e.g. ::ffff:1.2.3.4) is normalized to a 4-byte slice. The decoder (decodeRelayIP in client/internal/engine.go) already handles both 4- and 16-byte inputs correctly via netip.AddrFromSlice, which the function's docstring explicitly documents.

Optional nit: p.Credential is dereferenced unconditionally on line 84; if a future caller passes a zero CredentialPayload{} this will panic. A nil-guard returning a clear error would harden the API without changing happy-path behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@shared/signal/client/client.go` around lines 74 - 96, MarshalCredential
dereferences p.Credential without checking for nil which can panic if a caller
passes a zero CredentialPayload; add a nil-guard at the top of MarshalCredential
(the function in shared/signal/client/client.go) that checks if p.Credential ==
nil and returns nil plus a clear error (e.g., fmt.Errorf("missing credential in
CredentialPayload")) so callers get a descriptive error instead of a panic,
keeping the current return signature (*proto.Message, error).

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes